Repository navigation
Conversation
Both meta-tools were annotated destructiveHint: true, but neither deletes anything: they change which tools are exposed and can be reversed by the other. Set destructiveHint: false, keep idempotentHint: true, assert the annotations in tests and update the meta intent node. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZj3jC148r1VwBYogZpWKL
…lient PermissionAwareFastifyTransport connected the per-client server before the bundle's orchestrator finished preloading, so tools registered after connect and the client got 'Method not found' on tools/list. Await isReady() first, as FastifyTransport already does. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cv2mT1Y95UFmTFB8mZZhcL
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZj3jC148r1VwBYogZpWKL
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AKosQp8PUoZijAkZ7RWHj4
Disabling a toolset only updated state, so clients could still call its tools. DynamicToolManager now keeps the RegisteredTool handles returned by registerTool and calls remove() on disable, frees the names in ToolRegistry so the toolset can be enabled again, and rolls back a partially failed enable the same way. Intent layer, README and the meta-tool description no longer claim the SDK can't remove tools. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKeDY2JCSBC6vG56EEKziV
enableToolsets() reports failures in its result instead of throwing, so ServerOrchestrator.isReady() was true even when a startup toolset failed to enable, and FastifyTransport ignored the result anyway. A client could connect with only part of its tools. initializeToolsets() now turns a failed result into initError, and the transport answers initialize with a JSON-RPC -32603 error (HTTP 500) and drops the bundle so the next initialize retries. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJfs8sWB5EK8dXXjDU7Mv3
- Require the request Origin header to be a bare origin - Type allowedOrigins as readonly string[] - Export OriginGuard type instead of ReturnType<typeof ...> - Cover PermissionAwareFastifyTransport invalid-entry construction Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AKosQp8PUoZijAkZ7RWHj4
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKeDY2JCSBC6vG56EEKziV
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AKosQp8PUoZijAkZ7RWHj4
Reviewer findings: exposure-policy refusals and duplicate startup toolset names were made fatal by the previous commit. Only failures that happen while registering a toolset's tools (carried as a code on the enable result) now fail readiness. Changelog files the behaviour change as Breaking. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJfs8sWB5EK8dXXjDU7Mv3
…g entry Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJfs8sWB5EK8dXXjDU7Mv3
…to claude/project-thread-4ocdp6
…to claude/project-thread-4ocdp6
…to claude/project-thread-4ocdp6
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
…ets failed to load Mirrors FastifyTransport: a not-ready bundle gets a 500 / -32603 on initialize and is dropped from the cache. Docs updated for 0.8.0. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
TS review:
|
| # | Sev | Location | Finding |
|---|---|---|---|
| 1 | HIGH | CHANGELOG.md [0.8.0] Breaking, first bullet |
Says createPermissionBasedMcpServer clients with failing startup toolsets now get -32603 / HTTP 500. False: the permission orchestrator is built with toolsets: [], createPermissionAwareBundle enables toolsets itself, partial failure still connects (failedToolsets), total failure is 403, so isReady() never goes false there. |
| 2 | MEDIUM | CHANGELOG.md [0.8.0] Fixed (#30) |
Describes PermissionAwareFastifyTransport, which is not exported from src/index.ts, racing on background preload. Unreachable via the public factory, which awaits enableToolsets before returning. |
| 3 | MEDIUM | src/permissions/AGENTS.md flow; root AGENTS.md invariant 4 |
Imply the isReady / 500 path covers permission clients, and contradict "partial success continues with warning". |
| 4 | MEDIUM | tests/e2e/permissionBased.e2e.test.ts (new describes) |
Build the transport directly with hand-made bundles, so they pass while the public path is unchanged and hide finding 1. Label as transport-level, and pin the real behaviour through createPermissionBasedMcpServer. |
| 5 | LOW | README.md startup note |
"activates with no tools" overstates it; other modules in the toolset still register ("fewer tools"). |
{
"recommendation": "REQUEST_CHANGES",
"summary": "Version bump and most changelog/doc claims match the code, but the Breaking entry (and related docs/tests) claim createPermissionBasedMcpServer clients now get a 500/-32603 on failed startup toolsets, which cannot happen because that factory enables toolsets itself, tolerates partial failure, and leaves orchestrator.isReady() always true.",
"findings": [
{ "severity": "HIGH", "file": "CHANGELOG.md", "line": null, "issue": "Breaking entry claims permission-based clients get -32603/500 on failed startup toolsets; false for the public factory." },
{ "severity": "MEDIUM", "file": "CHANGELOG.md", "line": null, "issue": "Fixed entry for #30 describes an unexported class racing on preload; unreachable via createPermissionBasedMcpServer." },
{ "severity": "MEDIUM", "file": "src/permissions/AGENTS.md", "line": null, "issue": "Permission Flow and root invariant 4 imply the isReady/500 path covers permission clients and contradict partial-success behaviour." },
{ "severity": "MEDIUM", "file": "tests/e2e/permissionBased.e2e.test.ts", "line": null, "issue": "New e2e tests bypass the public factory and mask the changelog inaccuracy." },
{ "severity": "LOW", "file": "README.md", "line": null, "issue": "'activates with no tools' vs 'fewer tools' elsewhere." }
]
}Generated by Claude Code
createPermissionBasedMcpServer enables toolsets in its bundle factory and continues on partial failure, so the isReady() rejection does not apply. Pin that with a public-factory test and label the transport-level tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
Fixes for the round 1 TS reviewAll five findings were correct (I confirmed the permission flow in
The guard itself stays: it is a small mirror of Generated by Claude Code |
…ening Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
TS review:
|
| # | Sev | Location | Finding | Fix (1c58bb8) |
|---|---|---|---|---|
| 1 | LOW | CHANGELOG.md Fixed (#30) |
"hand-built createPermissionAwareBundle callback" implies a path consumers have; the class and helper are not exported. |
Reworded as internal hardening: the class is not public API and createPermissionBasedMcpServer is unchanged. |
| 2 | LOW | permissionBased.e2e.test.ts "all failed" |
Bare rejects.toThrow() passes on any error. |
Now asserts /Access denied/ (the 403 body). |
| 3 | LOW | permissionBased.e2e.test.ts partial failure |
Did not assert the failed toolset's tool is absent. | Now asserts dup.ping is not listed, which also pins the #31 rollback on the public path. |
{"recommendation":"COMMENT","summary":"All five round-1 findings fixed correctly and truthfully; only LOW polish remains.","findings":[{"severity":"LOW","file":"CHANGELOG.md","line":null,"issue":"#30 entry implies a user-reachable path; reword as internal hardening."},{"severity":"LOW","file":"tests/e2e/permissionBased.e2e.test.ts","line":null,"issue":"bare rejects.toThrow() in the all-failed test."},{"severity":"LOW","file":"tests/e2e/permissionBased.e2e.test.ts","line":null,"issue":"partial-failure test does not assert the failed toolset's tool is absent."}]}All three are addressed in 1c58bb8 (typecheck clean, 395 tests pass). Round 3 follows to confirm APPROVE.
Generated by Claude Code
TS review:
|
Requested by Ben · project thread
Before:
mainis at 0.7.0, and five finished changes (#29, #30, #31, #32, #33) are open as separate PRs with no release entry.After: version 0.8.0 with one changelog entry covering all five, plus one small addition:
PermissionAwareFastifyTransportnow rejects a client whose startup toolsets failed to load, the follow-up #33 and #30 asked for. Docs match the new behaviour. Nothing is tagged or published.Merge order (read first)
Base is
main, but none of #29-#33 has merged yet, so this draft branch contains all five merged in (one merge conflict, inCHANGELOG.md, resolved by writing the 0.8.0 entry). Merge them first, in any order; this PR's diff then shrinks to the release commits and merges cleanly on top. Do not rebase or force-push the PR branches.enable_toolset/disable_toolsetadvertisedestructiveHint: falseconnect()disable_toolsetremoves the toolset's toolshttp.allowedOriginsOrigin check on/mcpinitializeand STATIC startup (Breaking, still a draft)This release ships #33 as written (Strict semantics).
What the release adds beyond the five PRs
package.json/package-lock.json0.7.0 to 0.8.0, andCHANGELOG.md[0.8.0]with Breaking, Added, Changed and Fixed sections and migration notes.PermissionAwareFastifyTransport:isReady()false gives 500 / JSON-RPC-32603and drops the cached bundle, asFastifyTransportdoes. Two new e2e tests fail without it.AGENTS.mdinvariant 4 (loader errors stay silent, registration failures no longer do) andsrc/permissions/AGENTS.md.allowedOrigins, destructive hints and disable-removes-tools were already documented by their PRs.Verification
npm run typecheck,npm run test:run(393 tests) andnpm run buildpass.npm publish --dry-run: toolception@0.8.0, 64 files, 107 kB; onlydist/,package.json,README.md,CHANGELOG.mdandLICENSE; nosrc/, tests or env files.npm run verify:toolceptionin the FMP server (main, 2.7.0) against the packed tarball: 19/21. Both failures are expected and are server work that must merge first:ALL_TOOLSdoes not start: thecalendarandearningstoolsets both list theearnings-transcriptmodule, sogetLatestEarningsTranscriptscollides (src/constants/toolSets.ts). The server needs a catalog fix, being done in a separate project thread.should disable toolset (state only, tools remain)asserts the old disable behaviour. Fixed by server #113. (The same overlap breaks the legacy/all-tools smoke tests and the installation checks.)Server prerequisites before the FMP server takes 0.8.0
earnings-transcriptmodule from one ofcalendar/earnings.npm run verify:toolception -- --source 0.8.0must be green, then the dependency bump PR, with the docs that say disable is "state tracking only" (docs/USAGE.md,src/endpoints/server-card.ts) changed together with it.Release steps (Ben)
After the five PRs and this one are merged: tag
v0.8.0on main HEAD (release.yml publishes to npm and creates the GitHub Release). The command will be given in the project thread.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr